Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(rating): line-wrapping comments for grade_cap_reasons #2568

Closed
wants to merge 1 commit into from
Closed

feat(rating): line-wrapping comments for grade_cap_reasons #2568

wants to merge 1 commit into from

Conversation

magnuslarsen
Copy link
Contributor

Describe your changes

Fixes #2567

The grade_cap_reasons are now line-wrapped depending on terminal width

I've additional presumed you wanted the startTLS comment as a the grade-cap reason, and added it. It looks like this (with a narrow terminal):

image

And for non-startTLS but still broken (on a wide terminal):

image

What is your pull request about?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Typo fix
  • Documentation update
  • Update of other files

If it's a code change please check the boxes which are applicable

  • For the main program: My edits contain no tabs and the indentation is five spaces
  • I've read CONTRIBUTING.md and Coding_Convention.md
  • I have tested this fix against >=2 hosts and I couldn't spot a problem
  • I have tested this new feature against >=2 hosts which show this feature and >=2 host which does not (in order to avoid side effects) . I couldn't spot a problem
  • For the new feature I have made corresponding changes to the documentation and / or to help()
  • If it's a bigger change: I added myself to CREDITS.md (alphabetical order) and the change to CHANGELOG.md

@drwetter
Copy link
Owner

Thanks a lot, @magnuslarsen .

Not sure whether we should just take my comment into the description but it's definitely a good idea to explain the grade cap better. I'll amend that today or tomorrow.

@magnuslarsen
Copy link
Contributor Author

magnuslarsen commented Sep 18, 2024 •
edited
Loading

Feel free to change whatever :-) At least it wraps nicely on very long reasons

@magnuslarsen
Copy link
Contributor Author

magnuslarsen commented Sep 18, 2024 •
edited
Loading

Hmm, I am unsure why the CI/CD fails to output any grade reasons; running it on my laptop (also Ubuntu 22.04) towards a known good host results in:

$  ./testssl.sh --color 3 --ip=one https://google.com
[...]
 Rating (experimental)

 Rating specs (not complete)  SSL Labs's 'SSL Server Rating Guide' (version 2009q from 2020-01-30)
 Specification documentation  https://github.com/ssllabs/research/wiki/SSL-Server-Rating-Guide
 Protocol Support (weighted)  100 (30)
 Key Exchange     (weighted)  90 (27)
 Cipher Strength  (weighted)  90 (36)
 Final Score                  93
 Overall Grade                A
 Grade cap reasons            Grade capped to A. HSTS is not offered

 Done 2024-09-18 10:30:50 [ 117s] -->> 142.250.185.142:443 (google.com) <<--

@drwetter
Copy link
Owner

https://github.com/drwetter/testssl.sh/actions/runs/10917441970/job/30300762060

image

image

I am running it again..

@drwetter
Copy link
Owner

bear with me

@magnuslarsen
Copy link
Contributor Author

No rush - this is a minor fix

@drwetter
Copy link
Owner

drwetter commented Oct 14, 2024 •
edited
Loading

The problem is that the HTML output doesn't contain the grade cap reason, and I am scratching my head why.

Here are the relevant outputs again (see above) from HTML and screen which are used in ./t/32_isHTML_valid.t

 Rating (experimental)

 Rating specs (not complete)  SSL Labs&apos;s &apos;SSL Server Rating Guide&apos; (version 2009q from 2020-01-30)
 Specification documentation  <a href="https://github.com/ssllabs/research/wiki/SSL-Server-Rating-Guide" style="color:black;text-decoration:none;">https://github.com/ssllabs/research/wiki/SSL-Server-Rating-Guide</a>
 Protocol Support (weighted)  97 (29)
 Key Exchange     (weighted)  90 (27)
 Cipher Strength  (weighted)  90 (36)
 Final Score                  92
 Overall Grade                B
 Grade cap reasons
 Done 2024-10-12 21:40:07 [0054s] --&gt;&gt; 193.99.144.80:443 (heise.de) &lt;&lt;--


</pre>
</body>
</html>

Rating (experimental)

 Rating specs (not complete)  SSL Labs's 'SSL Server Rating Guide' (version 2009q from 2020-01-30)
 Specification documentation  https://github.com/ssllabs/research/wiki/SSL-Server-Rating-Guide
 Protocol Support (weighted)  97 (29)
 Key Exchange     (weighted)  90 (27)
 Cipher Strength  (weighted)  90 (36)
 Final Score                  92
 Overall Grade                B
 Grade cap reasons            Grade capped to B. TLS 1.1 offered
                              Grade capped to A. HSTS max-age is too short

 Done 2024-10-12 21:40:07 [0054s] -->> 193.99.144.80:443 (heise.de) <<--

@drwetter
Copy link
Owner

Just to make sure: The problem is not the check, it is the code change which seems to swallow the grade cap reasons in HTML. 🧐

@magnuslarsen
Copy link
Contributor Author

magnuslarsen commented Oct 14, 2024 •
edited
Loading

Aah good catch! out_row_aligned_max_width() is calling tm_out() under the hood, which is not outputting to HTML

Looking at out() and outln(), they end up calling html_out "$(html_reserved "$1")" as well - so is the solution here to call both out_row_aligned_max_width() and html_out()?

That is:

          if [[ $reason_nr -eq 0 ]]; then
               pr_bold " Grade cap reasons            "
               out_row_aligned_max_width "$reason\n" '                                ' $TERM_WIDTH
               html_out "$(html_reserved "$reason")"
          else
               out_row_aligned_max_width "                              $reason\n" '                                ' $TERM_WIDTH
               html_out "$(html_reserved "$reason")"
          fi

I'm not sure it's prettier to put it all on the same, already long line?

@drwetter
Copy link
Owner

I was just wondering why that problem didn't show up before.

At the moment I am not sure yet what the best solution is . The fastest would be to leave that on one line but it's kind of ugly. For a more sustainable fix I'd like to understand whether we have a bug which has not been discovered yet by the unit test -- i.e. another long line not showing up in HTML output.

@drwetter drwetter self-assigned this Oct 14, 2024
drwetter added a commit that referenced this pull request Oct 14, 2024
@drwetter
Fix HTML output in #2568
7c0ccb3
@drwetter
Copy link
Owner

It works if one uses outln $( out_row_aligned_max_width() )" instead. I merged your proposal into #2579 and work on the details.

Thanks!

@drwetter drwetter closed this Oct 14, 2024
@magnuslarsen
Copy link
Contributor Author

Thanks Dirk :-)

@dcooper16 dcooper16 mentioned this pull request Oct 15, 2024
Open
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@drwetter drwetter

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature request] line wrapping for grade cap
2 participants
@magnuslarsen @drwetter